-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix invalid connect options, when using browserURL #212
fix invalid connect options, when using browserURL #212
Conversation
i have added some hardening into teardown, so it can handle most cases of dirty env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway I'm not sure if it' is an exact solution to the issue.
await this.global.context.close() | ||
if (page) { | ||
page.removeListener('pageerror', handleError) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are checking if (page)
on line L120 and L129. If it's supposed to be run only if page
is true
you can combine those two if blocks into one. Logic would stay the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done always, if page is set. Independant of what we want to close.
} else { | ||
await this.global.page.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh logic would stay the same if you will change lines L120-L132 to:
if (puppeteerConfig.browserContext === 'incognito' && context) {
await context.close()
} else if (page) {
page.removeListener('pageerror', handleError)
await page.close()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is different: If browserContent === 'incognito' && context, then page.removeListener, wont be called, but that call should only depend on existance of page.
So we must move the page.removeListener call into its own block, leaving us with this:
if (puppeteerConfig.browserContext === 'incognito' && context) {
await context.close()
} else if (page) {
await page.close()
}
Normally this also changes the logic of the original block i posted, but since page and context depend on each other, so when browserContent === 'incognito' && context === undefined (because error in setup), it must be page === undefined, because there cant be a page without context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To expand on this: You must have knowledge about the dependency between context & page, to correctly understand the minified if/else you propose, which makes it harder to reason about what happens.
i have fixed the linting error, but imho in this special case, it decreases the readability of the code, because the inner if is independant to the outer if/else. |
Hello @backbone87, thanks for the fix, it looks better than before and it should fix the bug. I will merge it and release a new version. |
@neoziro can we have a backport to v3 for this? |
@backbone87 yes we can |
Published! |
Summary
Specifying both options
browserURL
andbrowserWSEndpoint
is invalid when calling connect. SincePuppeteerEnvironment
always gets a WS endpoint prepared from global setup, we must unset thebrowserURL
option.Edit: Since this problem also exists in v3, it would be nice to backport this, since v4 does not support jest v23 anymore
Edit2: This most likely fixes #200, at least my occurence of it. The real problem is that setup exceptions get swallowed, if teardown also throws. Guessing that the jest code looks something like this:
So the actual problem (setup failing) gets shadowed by the teardown fragility not being able to do its work on a dirty env.